First pass atAchievements contract. #357
Conversation
zomglings
left a comment
zomglings
left a comment
There was a problem hiding this comment.
Some questions/comments.
| IERC721 nft = IERC721(istore.ContractERC721Address); | ||
|
|
||
| require( | ||
| msg.sender == nft.ownerOf(subjectTokenID), |
There was a problem hiding this comment.
Do we want this? Don't we want it so that we can mint achievements to an NFT? We won't be owners of that NFT.
There was a problem hiding this comment.
I see, you are leaning on adminBatchMintToInventory for that. I guess I'm ok with this. Wouldn't mind it being permissionless, though, as long as the msg.sender has a valid signature.
| .EquippedItems[istore.ContractERC721Address][subjectTokenId][poolId] | ||
| .ItemType != 0 | ||
| ) { | ||
| _unequip(subjectTokenId, poolId, true, 0); |
There was a problem hiding this comment.
Since the slots are persistent, wouldn't this revert anyway? Or is the behavior different for the internal _unequip method? Or different for players and admins?
There's a lot of reasoning that goes into understanding what happens here.
I'd rather we had an explicit reversion. Perhaps we need a method that helps us rescue ourselves from erroneously granting achievements?
No description provided.